-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #508: Aggregate data class #513
Conversation
09104ae
to
2845b47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a self review this looks good to me aside from the comments I have posted and the testing gaps (see the codecov report) which need closing before this can move out of draft.
I also see that the pkgdown
site is failing due to the Ebola vignette failing in the postprocessing. This works locally so is a bit of a poser but needs fixing before moving forward with this.
I have now resolved all issues highlighted in a self-review so the only work that now needs doing here is to plug the remaining testing gaps for the new data format and chase down the pkgdown issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another self-review. All looking good to me now. Waiting on code coverage and still need to resolve the pkgdown CI issue in the Ebola vignette
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #513 +/- ##
==========================================
+ Coverage 92.05% 94.38% +2.32%
==========================================
Files 16 17 +1
Lines 743 837 +94
==========================================
+ Hits 684 790 +106
+ Misses 59 47 -12 ☔ View full report in Codecov by Sentry. |
Unit tests are now in place covering the new functionality. Remaining small gaps are false positives from previous uncovered code. |
Not evaluating the code for the failing plot section didn't help - it just shifted the error to the next plotting block. Usually this means that there is a problem upstream (which doesn't cause a error) or it could be a very oddly masked dependency issue. I don't currently see any issue locally. Closing out all the final plotting blocks leads to the build succeeding. The code inside the blocks that isn't plotting code works as expected on CI. This means the issue is localised within the plotting code (and repeated across both blocks). |
Doing some searching and there is some suggestion this could be a cache issue (due to lazy caching being unstable with large objects). This makes some sense as the introduction of truncation here and removal of filtering the data has increased the potential size of the object quite a bit. |
It looks like this was indeed a cache issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final self review. I see all checks passing and everything added covered by docs and tests. Functionality is as described in the target issue and support is present for all implemented models.
Description
This PR closes #508 and #412 by adding an aggregate data class and finalising support in
as_epidist_marginal_model
.Checklist